Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PokemonGo bot version to docker image #4884

Closed
wants to merge 2 commits into from

Conversation

walkerlee
Copy link
Contributor

@walkerlee walkerlee commented Aug 29, 2016

Short Description:

add commit version when create docker image

@mention-bot
Copy link

@walkerlee, thanks for your PR! By analyzing the annotation information on this pull request, we identified @th3w4y, @Gobberwart and @alexyaoyang to be potential reviewers

&& cat /tmp/$BUILD_BRANCH.tar.gz | tar -zxf - --strip-components=1 -C /usr/src/app \
&& apk del .tar-deps \
&& rm /tmp/$BUILD_BRANCH.tar.gz
ADD https://api.github.com/repos/$BUILD_REPO/git/refs/heads/$BUILD_BRANCH /tmp/pgobot-version
Copy link
Contributor

@th3w4y th3w4y Aug 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not working if i specify a particular commit as the BUILD_BRANCH
Ex:
https://api.github.com/repos/PokemonGoF/PokemonGo-Bot/git/refs/heads/1862b9bf296e201b1e3b5f2783f6a7436ced5698 404

{
"message": "Not Found",
"documentation_url": "https://developer.github.com/v3"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any situation we need to use a commit?

Copy link
Contributor

@th3w4y th3w4y Aug 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it order to test new changes... that is why you can also override the BUILD_REPO

In the past there was a COPY . /usr/src/app

the user was able to do it's changes and docker build again

I replaced that with ADD https://github.com/$BUILD_REPO/archive/$BUILD_BRANCH.tar.gz /tmp for various reasons:

  • security COPY . means including auth and unfits... and stuff (I know there is a .dockerignore but whatever... )
  • being able to reproduce the build if you know create date and LABEL build_branch and build_repo and not support some uncommited local modification that the user might have done...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so I'll delete this PR

@Gobberwart
Copy link
Contributor

I was identified as a reviewer but I don't use docker so pass.

@th3w4y
Copy link
Contributor

th3w4y commented Aug 29, 2016

@Gobberwart and now that you commented on this PR the probability to be identified next time increased 🎉

@walkerlee walkerlee closed this Aug 29, 2016
@Gobberwart
Copy link
Contributor

LOL and now you commented....

OK I think we're done :D

@th3w4y
Copy link
Contributor

th3w4y commented Aug 29, 2016

version can always be extrapolated from:

docker inspect --format='{{.Created}} {{.ContainerConfig.Labels}}' container_tag_or_id

then use the info and get the commit via git-rev-parse

Example:

git rev-parse '@{2016-08-25 18:30:30}'
9605728255f996f156fd00b1467d7f14afb1d3a4

@walkerlee
Copy link
Contributor Author

@th3w4y I think this way isn't very precisely

@th3w4y
Copy link
Contributor

th3w4y commented Aug 29, 2016

@walkerlee I agree (there could be a few seconds raise condition.. ) but is the only idea i have for now

@th3w4y
Copy link
Contributor

th3w4y commented Aug 29, 2016

@walkerlee
Copy link
Contributor Author

@th3w4y
LGTM, thank you!

@th3w4y
Copy link
Contributor

th3w4y commented Aug 29, 2016

LABEL BUILD_COMMIT $(curl -H "Accept: application/vnd.github.VERSION.sha" https://api.github.com/repos/$BUILD_REPO/commits/$BUILD_BRANCH)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants